Updates tar distribution to conform to changes in install demo configuration script in security plugin #4250

DarshitChanpura · 2023-11-29T20:46:45Z

Description

Following up on the changes to demo configuration script in security plugin, opensearch-project/security#3669, the OS supported distributions need to adapt to changes in the behavior of demo configuration script.

What has changed?

The ./opensearch-tar-install.sh will now have these execution paths:

OPENSEARCH_INITIAL_ADMIN_PASSWORD value is set to a strong password: Demo configuration script execution succeeds
If weak password is provided in either of the cases, tar-install.sh execution will fail.
If no password is provided, the script will exit and installation will fail.

For detailed testing output, refer to this comment on the issue mentioned below.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…uration script in security plugin Signed-off-by: Darshit Chanpura <[email protected]>

codecov · 2023-11-29T20:50:56Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (31cc1ef) 91.35% compared to head (840a220) 91.35%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4250   +/-   ##
=======================================
  Coverage   91.35%   91.35%           
=======================================
  Files         190      190           
  Lines        6175     6175           
=======================================
  Hits         5641     5641           
  Misses        534      534

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

scripts/startup/tar/linux/opensearch-tar-install.sh

rishabh6788 · 2023-11-29T21:23:54Z

Just to be clear, the user is still admin and only the password has to be provided, right?

DarshitChanpura · 2023-11-30T01:15:22Z

Just to be clear, the user is still admin and only the password has to be provided, right?

Yes, correct.

prudhvigodithi · 2023-12-05T19:08:07Z

Adding @derek-ho, coming from the issue #4094, looks this change is having a problem to deal multi node cluster? Derek can you please add more details. Thank you

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2023-12-11T14:34:52Z

Adding @derek-ho, coming from the issue #4094, looks this change is having a problem to deal multi node cluster? Derek can you please add more details. Thank you

This issue has been addressed now by skipping password generation and quitting the script if no custom password was provided.

dblock · 2023-12-13T16:59:34Z

Is the variable really initialAdminPassword?! All OpenSearch variables are OPENSEARCH_SOMETHING_SOMETHING, where did we deviate from this convention?

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2023-12-13T20:08:44Z

@dblock Updated the info statements and the corresponding PR: opensearch-project/security#3843
has been raised in security plugin

DarshitChanpura · 2023-12-14T21:07:22Z

@prudhvigodithi @rishabh6788 Would you please review this?

Signed-off-by: Darshit Chanpura <[email protected]>

rishabh6788 · 2023-12-14T21:19:36Z

I am okay with approving this change but we cannot merge this until security plugin changes have been back-ported to 2.x as it will start breaking 2.12.0 tests and benchmark runs.

DarshitChanpura · 2023-12-14T21:49:51Z

I am okay with approving this change but we cannot merge this until security plugin changes have been back-ported to 2.x as it will start breaking 2.12.0 tests and benchmark runs.

The backport PR is currently waiting review: opensearch-project/security#3845

DarshitChanpura · 2024-01-02T14:50:14Z

opensearch-project/security#3845 is merged and this PR is unblocked.

peterzhuamazon · 2024-01-02T20:59:54Z

scripts/startup/tar/linux/opensearch-tar-install.sh

+        echo "OpenSearch 2.12.0 onwards, the OpenSearch Security Plugin introduces a change that requires an initial password for 'admin' user."
+        echo "Please define an environment variable 'OPENSEARCH_INITIAL_ADMIN_PASSWORD' with a strong password string."
+        echo "If a password is not provided, the setup will quit."
+        bash $OPENSEARCH_HOME/plugins/opensearch-security/tools/install_demo_configuration.sh -y -i -s || exit 1


After talking to @prudhvigodithi we should remove the exit 1, and add set -e to the install script, as the original design removed that due to chmod 777 /dev/shm, which has been removed some time ago.

More conversation with @DarshitChanpura and seems like set -e might have more impact on docker side. He will get more info on that and we will need some more discussion on it with @prudhvigodithi.

I am ok with either approach, just think it would be better if we are consistent across all distributions.

set -e affects the entire file setup and analyzing that change is out of scope, IMO. @prudhvigodithi @peterzhuamazon please lmk if we should indeed use set -e. TAR is much simple to analyze and can be changed to use set -e, however, docker setup is bit more involved in terms of understanding all possible code paths. Windows however doesn't have set -e from what I understand, and hence will require some form of || exit /b 1.

So, for consistency purposes I'd vote on keeping it as is with || exit 1. This has been extensively tested on TAR, docker and Windows, and the setup works as expected with this change.

@prudhvigodithi @peterzhuamazon @rishabh6788 thoughts?

DarshitChanpura · 2024-01-08T15:41:21Z

@peterzhuamazon @prudhvigodithi @rishabh6788 Could I please get reviews on this?

peterzhuamazon · 2024-01-08T22:02:38Z

Also need @prudhvigodithi to confirm on @DarshitChanpura comment above regarding keeping the exit 1 for docker and tar, but remove for deb and rpm.

DarshitChanpura · 2024-01-09T14:37:09Z

Also need @prudhvigodithi to confirm on @DarshitChanpura comment above regarding keeping the exit 1 for docker and tar, but remove for deb and rpm.

This PR only addresses TAR ball, no docker/windows/rpm/deb changes are present

DarshitChanpura · 2024-01-11T16:43:27Z

@peterzhuamazon Could I get some re-reviews on this PR?

prudhvigodithi · 2024-01-11T18:27:37Z

I'm ok with exit 1, even though having set -e is better as the execution codes are directly dictated by install_demo_configuration.sh but needs proper testing on how the script would behave (using set -e) with docker etc.
@peterzhuamazon add your final thought.

prudhvigodithi · 2024-01-11T18:39:17Z

@DarshitChanpura can you please use as echo -e "This is a line with a new line.\nThis is the second line." to print statements in a new line. We dont need 3 echo lines.

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2024-01-11T18:49:50Z

echo -e maybe slightly unstable. I've used printf instead which is more stable for combining 3 lines with \n

DarshitChanpura · 2024-01-16T15:14:38Z

@peterzhuamazon @prudhvigodithi @rishabh6788 Could I get some more reviews?

peterzhuamazon · 2024-01-16T18:22:17Z

scripts/startup/tar/linux/opensearch-tar-install.sh

@@ -10,7 +10,8 @@ cd $OPENSEARCH_HOME
 KNN_LIB_DIR=$OPENSEARCH_HOME/plugins/opensearch-knn/lib
 ##Security Plugin
 if [ -d "$OPENSEARCH_HOME/plugins/opensearch-security" ]; then
-        bash $OPENSEARCH_HOME/plugins/opensearch-security/tools/install_demo_configuration.sh -y -i -s
+        printf "OpenSearch 2.12.0 onwards, the OpenSearch Security Plugin introduces a change that requires an initial password for 'admin' user. \nPlease define an environment variable 'OPENSEARCH_INITIAL_ADMIN_PASSWORD' with a strong password string. \nIf a password is not provided, the setup will quit."


echo -e should be used here to be consistent instead of printf.

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura · 2024-01-16T20:44:44Z

@peterzhuamazon is there anything blocking this PR still?

Updates tar distribution to conform to changes in install demo config…

142590d

…uration script in security plugin Signed-off-by: Darshit Chanpura <[email protected]>

github-actions bot added the valued-contributor label Nov 29, 2023

peterzhuamazon reviewed Nov 29, 2023

View reviewed changes

scripts/startup/tar/linux/opensearch-tar-install.sh Outdated Show resolved Hide resolved

DarshitChanpura marked this pull request as ready for review November 30, 2023 15:43

DarshitChanpura requested review from dblock, bbarani, gaiksaya, rishabh6788, zelinh, jordarlu, prudhvigodithi, Divyaasm and tianleh as code owners November 30, 2023 15:43

Merge remote-tracking branch 'upstream/main' into tar-dist-update

0b2a10f

Updates log statement

7b37d44

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura mentioned this pull request Dec 8, 2023

Updates Docker and windows distribution build files to reflect to changes in admin user setup #4274

Merged

Updates info log

78f3345

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added 2 commits December 13, 2023 15:04

Merge remote-tracking branch 'upstream/main' into tar-dist-update

595bb91

Updates the info message to reflect env variable name change

bc713dd

Signed-off-by: Darshit Chanpura <[email protected]>

Corrects info logs

d50eff1

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the tar-dist-update branch from 93cdf8c to d50eff1 Compare December 14, 2023 21:12

Captializes security plugin name in info logs

433c0bc

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura force-pushed the tar-dist-update branch from fdab519 to 433c0bc Compare December 14, 2023 21:16

peterzhuamazon requested changes Jan 2, 2024

View reviewed changes

This was referenced Jan 4, 2024

[Enhancement] Ensure graceful termination if weak/no initial admin password set opensearch-project/security#3915

Closed

[META] Update all instances of admin:admin across different repositories opensearch-project/security#3624

Closed

DarshitChanpura added 2 commits January 11, 2024 13:43

Merge remote-tracking branch 'upstream/main' into tar-dist-update

6c76e91

Updates multiple echos with a single printf

805c86f

Signed-off-by: Darshit Chanpura <[email protected]>

DarshitChanpura added the v2.12.0 label Jan 16, 2024

prudhvigodithi approved these changes Jan 16, 2024

View reviewed changes

peterzhuamazon reviewed Jan 16, 2024

View reviewed changes

DarshitChanpura added 2 commits January 16, 2024 13:48

Merge remote-tracking branch 'upstream/main' into tar-dist-update

1f4b6b0

Change to echo -e

840a220

Signed-off-by: Darshit Chanpura <[email protected]>

peterzhuamazon approved these changes Jan 16, 2024

View reviewed changes

peterzhuamazon merged commit f37c31c into opensearch-project:main Jan 16, 2024
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates tar distribution to conform to changes in install demo configuration script in security plugin #4250

Updates tar distribution to conform to changes in install demo configuration script in security plugin #4250

DarshitChanpura commented Nov 29, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

rishabh6788 commented Nov 29, 2023 •

edited

Loading

DarshitChanpura commented Nov 30, 2023

prudhvigodithi commented Dec 5, 2023 •

edited

Loading

DarshitChanpura commented Dec 11, 2023

dblock commented Dec 13, 2023

DarshitChanpura commented Dec 13, 2023 •

edited

Loading

DarshitChanpura commented Dec 14, 2023

rishabh6788 commented Dec 14, 2023

DarshitChanpura commented Dec 14, 2023 •

edited

Loading

DarshitChanpura commented Jan 2, 2024

peterzhuamazon Jan 2, 2024

peterzhuamazon Jan 3, 2024

DarshitChanpura Jan 4, 2024

DarshitChanpura commented Jan 8, 2024

peterzhuamazon commented Jan 8, 2024 •

edited

Loading

DarshitChanpura commented Jan 9, 2024 •

edited

Loading

DarshitChanpura commented Jan 11, 2024

prudhvigodithi commented Jan 11, 2024

prudhvigodithi commented Jan 11, 2024 •

edited

Loading

DarshitChanpura commented Jan 11, 2024

DarshitChanpura commented Jan 16, 2024

peterzhuamazon Jan 16, 2024 •

edited

Loading

DarshitChanpura commented Jan 16, 2024

Updates tar distribution to conform to changes in install demo configuration script in security plugin #4250

Updates tar distribution to conform to changes in install demo configuration script in security plugin #4250

Conversation

DarshitChanpura commented Nov 29, 2023 • edited Loading

Description

What has changed?

codecov bot commented Nov 29, 2023 • edited Loading

Codecov Report

rishabh6788 commented Nov 29, 2023 • edited Loading

DarshitChanpura commented Nov 30, 2023

prudhvigodithi commented Dec 5, 2023 • edited Loading

DarshitChanpura commented Dec 11, 2023

dblock commented Dec 13, 2023

DarshitChanpura commented Dec 13, 2023 • edited Loading

DarshitChanpura commented Dec 14, 2023

rishabh6788 commented Dec 14, 2023

DarshitChanpura commented Dec 14, 2023 • edited Loading

DarshitChanpura commented Jan 2, 2024

peterzhuamazon Jan 2, 2024

Choose a reason for hiding this comment

peterzhuamazon Jan 3, 2024

Choose a reason for hiding this comment

DarshitChanpura Jan 4, 2024

Choose a reason for hiding this comment

DarshitChanpura commented Jan 8, 2024

peterzhuamazon commented Jan 8, 2024 • edited Loading

DarshitChanpura commented Jan 9, 2024 • edited Loading

DarshitChanpura commented Jan 11, 2024

prudhvigodithi commented Jan 11, 2024

prudhvigodithi commented Jan 11, 2024 • edited Loading

DarshitChanpura commented Jan 11, 2024

DarshitChanpura commented Jan 16, 2024

peterzhuamazon Jan 16, 2024 • edited Loading

Choose a reason for hiding this comment

DarshitChanpura commented Jan 16, 2024

DarshitChanpura commented Nov 29, 2023 •

edited

Loading

codecov bot commented Nov 29, 2023 •

edited

Loading

rishabh6788 commented Nov 29, 2023 •

edited

Loading

prudhvigodithi commented Dec 5, 2023 •

edited

Loading

DarshitChanpura commented Dec 13, 2023 •

edited

Loading

DarshitChanpura commented Dec 14, 2023 •

edited

Loading

peterzhuamazon commented Jan 8, 2024 •

edited

Loading

DarshitChanpura commented Jan 9, 2024 •

edited

Loading

prudhvigodithi commented Jan 11, 2024 •

edited

Loading

peterzhuamazon Jan 16, 2024 •

edited

Loading